Skip to content

Conversation

@brettfo
Copy link
Member

@brettfo brettfo commented May 16, 2018

This will enable nightly VSIX packages to ship with compiler changes as well as F5. Below is a screenshot of the F5 scenario showing that for both legacy and SDK projects the compiler used came from the VSIX instead of Program Files.

compiler

@brettfo brettfo requested a review from KevinRansom May 16, 2018 19:27
@TIHan
Copy link
Contributor

TIHan commented May 16, 2018

This is absolutely fantastic.

@brettfo brettfo merged commit 369c538 into dotnet:master May 16, 2018
@brettfo brettfo deleted the editor-compiler branch May 16, 2018 20:43
@Pilchie
Copy link
Member

Pilchie commented May 17, 2018

Wait - doesn't this mean that all builds in VS will use this compiler, even in shipped products? Is that what we want? Do customer environment variables or other properties override this or vice versa.

This seems like a pretty significant change to take without verifying some more scenarios for it first.

@dsyme
Copy link
Contributor

dsyme commented May 17, 2018

@brettfo Can you also check that the F# Interactive (fsi.exe, fsiAnyCpu.exe) that run in Visual Studio are the ones from the VSIX? thanks

@cartermp
Copy link
Contributor

@brettfo is this true?

doesn't this mean that all builds in VS will use this compiler, even in shipped products?

I would argue that we don't want this for shipping, as we want to be deliberate about compiler changes for shipped products.

Getting these out in nightlies is fantastic since it'll have more people banging on changes before they make it out, but we shouldn't be delivering compiler changes unless we rev versions of the compiler (and do so deliberately).

@brettfo
Copy link
Member Author

brettfo commented May 17, 2018

@Pilchie @cartermp: Even without this change we had no mechanism to ship only the IDE component; every VS insertion we do contains both IDE and compiler changes. True, builds from the IDE will now use this compiler, but it's identical to the one that was getting picked up from Program Files (x86)\... so there's no functional difference.

Once I get sign-off on #4819 we can refactor our VS insertion code to only insert one or the other (or both) and refactor the installed package to not have a compiler, but in the meantime nothing has changed with what compiler the developer ends up getting.

@Pilchie
Copy link
Member

Pilchie commented May 17, 2018

I'm still 👎 on this change. There are too many risks for users who set those variables themselves. I do not think this should be in the shipping code.

@cartermp
Copy link
Contributor

cartermp commented May 17, 2018

Another angle here is that we've received a lot of feedback over time that nightlies should contain latest compiler bits. People who wanted to test out new features earlier on didn't have a way to do that unless they built this repo themselves.

It sounds like this will not work in conjunction with the FSharp.Compiler.Tools package, which some people use. However, we don't officially support this scenario beyond obvious issues (e.g., #3222 where this exposed the fact that the legacy project system wasn't kicking off design-time builds properly).

We'll also incur a "you moved my cheese" issue when we move away from an MSI for the compiler as well (unless we also deploy the compiler to Program Files).

@brettfo
Copy link
Member Author

brettfo commented May 17, 2018

@dsyme fsi.exe and fsiAnyCpu.exe both run from the VSIX/F5 location.

@brettfo
Copy link
Member Author

brettfo commented May 17, 2018

@Pilchie What danger do see? The properties set in this PR happen at project creation so anything the developer sets in a .props/.targets/.fsproj will override what I set giving them their expected behavior.

Edit: turns out this isn't the case, but is addressed in #4934.

@KevinRansom
Copy link
Contributor

@Pilchie your issue is this?

        [ "FSharpPropsShim", "Microsoft.FSharp.NetSdk.props" 
          "FSharpTargetsShim", "Microsoft.FSharp.NetSdk.targets" 
          "FSharpOverridesTargetsShim", "Microsoft.FSharp.Overrides.NetSdk.targets" ] 

I think this is fine, because it is the initial state of these values prior to the project being run. If the developer adds a target to his project that forces these values to a different location then nothing is altered by us having precomputed the value.

We actually do this in our tests to pick a specific compiler implementation: https://github.com/Microsoft/visualfsharp/blob/master/FSharpTests.Directory.Build.props#L23

Because we want it to use a versy specific compiler it isn't guarded by a conditions, and so behaves as the tests would expect. It will pick a different compiler.

.

@Pilchie
Copy link
Member

Pilchie commented May 17, 2018

What if the user sets it via env var?

@brettfo
Copy link
Member Author

brettfo commented May 17, 2018

@KevinRansom and I have been digging into it and there is an issue in that the property we're setting can't be overwritten. I'm working on a change now to use the Roslyn approach of setting FSharpCompilerLocation (or whatever) and keying off of that in our shim targets. The upside is that if a user builds with /p:FSharpTargetsPath=... or via an environment variable, everything will work as expected. The downside is that consuming the VSIX compiler won't work until we've shipped the shims at least once, e.g., 15.8 RTM.

@abelbraaksma
Copy link
Contributor

abelbraaksma commented May 18, 2018

From a user standpoint, we've often been hit by either of these scenarios:

  • Downloading a Preview VS IDE, with bugs, only to find out that if you switch back to and RTM VS IDE, the new bugs are there too
  • Trying out changes from a PR or in the nightlies: if you don't have a VM (or just find setup/teardown cumbersome), it is pretty tough to find out how to do that without breaking any of your current installations (and even if you're convinced you do it right, any bug you'll find since, you'll be wondering whether you broke something by mixing installations on one system).

It is my main concern whenever I report a bug, or when others cannot repro it. Having the compiler, or essentially everything from the F# tooling in a VSIX and being able to install / use it with a specific VS instance would be a dream come true.

I've been working a while ago on an add/remove/clean installer helper for VSIX and easier managing it across different named instances from a batch file installation process. If all's in a VSIX, this may come in handy to try out new stuff: run the batch file (it'll make a shadow copy of your current situation, settings etc), try out the changes, revert everything to the previous situation.

Just saying: please don't let us wait until 15.8. Being able to get involved in the open source effort more easily and with less worries would mean the world to many people.

nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Jan 26, 2022
Add the compiler to the regular IDE package.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants